Implement flat_map operator by CorentinBT · Pull Request #374 · AlexInLog/ReactivePlusPlus

CorentinBT · 2023-06-10T10:31:28Z

Should I also implement a variant taking a ResultSelector like in rxcpp?

CorentinBT · 2023-06-10T10:32:46Z

+    {
+        copy_count_tracker verifier{};
+        auto obs = rpp::source::just<TestType>(std::move(verifier))
+                 | rpp::ops::flat_map([](copy_count_tracker&& verifier) { return rpp::source::just<TestType>(std::move(verifier)); });


@victimsnino do you know why sources don't move objects through operators? Preventing me from taking verifier as an rvalue ref here

Because it would be moved IF possible, but no guarantees. If it would be only pure rvalue, then it means second repeated subscribe would see nothing :)

I see, I added some cases for copies then

if you would use create with std::move inside, then it would be moved inside lambda

AlexInLog · 2023-06-10T15:02:49Z

+            TEST_RPP([&]()
+            {
+                rpp::source::create<int>([](const auto& obs){ obs.on_next(1); })
+                    | rpp::operators::flat_map([](int v) { return rpp::source::just(v * 2); })


Could you use “create” there instead of just to measure there only “flat_map”, not “just” ? :)

codecov · 2023-06-10T15:03:22Z

Codecov Report

Merging #374 (45e0ec6) into v2 (05a4d05) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##               v2     #374      +/-   ##
==========================================
+ Coverage   99.12%   99.13%   +0.01%     
==========================================
  Files          37       38       +1     
  Lines         802      812      +10     
==========================================
+ Hits          795      805      +10     
  Misses          7        7

Impacted Files	Coverage Δ
src/rpp/rpp/operators/flat_map.hpp	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

AlexInLog · 2023-06-10T15:04:19Z

+    RPP_NO_UNIQUE_ADDRESS Fn m_fn;
+
+    template<rpp::constraint::observable TObservable>
+        requires std::invocable<Fn, rpp::utils::extract_observable_type_t<TObservable>>


There is also invocation result must be observable

It is already required here isn't it enough?

No, check this comment

https://github.com/victimsnino/ReactivePlusPlus/pull/374/files#r1225415597

AlexInLog · 2023-06-10T15:05:10Z

+    auto operator()(TObservable&& observable) &&
+    {
+        return std::forward<TObservable>(observable)
+                | rpp::ops::map(std::forward<Fn>(m_fn))


Should be move

AlexInLog · 2023-06-10T15:07:47Z

 auto map(Fn&& callable);

+template<typename Fn>
+    requires rpp::constraint::observable<std::invoke_result_t<Fn, utils::convertible_to_any>>


Add check over !utils::is_not_template_callable as in map. It needed when passed function is purely template. For example [](auto v) {return rpp::source::just(v); } wouldn't satisfy your requirement

done, added a test too

AlexInLog · 2023-06-10T17:59:12Z


-    template<rpp::constraint::observable TObservable>
-        requires std::invocable<Fn, rpp::utils::extract_observable_type_t<TObservable>>
+    template<rpp::constraint::observable TObservable, typename ValueType = rpp::utils::extract_observable_type_t<TObservable>>


I see your attempt with ValueType to avoid duplicates, but it would affect final binaries due to it would be compiled-in as template function with 2 params instead of one =C it is better to create special concept instead

AlexInLog · 2023-06-10T18:01:22Z

                CHECK(mock.get_on_error_count() == 1);
            }
        }
+        SECTION("subscribe using flat_map with templated lambda")


i think, you can just place it in a748a6f#diff-4ad61bca345fc2ef117d5617e164dca71340d801c636340f2c051a5bd4e6e9d2R37

AlexInLog · 2023-06-10T18:02:42Z

+        auto obs = rpp::source::create<copy_count_tracker>([verifier = std::move(verifier)](const auto& obs) { obs.on_next(verifier); })
+                 | rpp::ops::map([](copy_count_tracker verifier) { return std::move(verifier); }) // copy from source to map


try instead

auto obs = rpp::source::create<copy_count_tracker>([verifier = std::move(verifier)](const auto& obs) { obs.on_next(std::move(verifier)); })

Though I subscribe two times here, so verifier will be in an invalid state in second subscription as it was already moved no?

I changed to capture by ref and pass by value

Though I subscribe two times here, so verifier will be in an invalid state in second subscription as it was already moved no?

No, verifier is special class that is valid after move =)

AlexInLog · 2023-06-11T15:42:38Z

+                CHECK(mock.get_on_error_count() == 0);
+            }
+        }
+        SECTION("subscribe using flat_map with error in middle")


Could you also add test over never in middle? to check that it is actually map + merge, not map + concat =)

AlexInLog · 2023-06-11T15:49:21Z

+    }
+}
+
+TEST_CASE("flat_map copies/moves")


I think, you can remove this test_case due to you are trying to test map + test merge at the same time, it is pretty hard. It is better to have corresponding tests to map and merge. I'm going to add them later.

yeah agree, done

AlexInLog · 2023-06-11T16:51:28Z

+    template<rpp::constraint::observable TObservable>
+        requires (std::invocable<Fn, rpp::utils::extract_observable_type_t<TObservable>> 
+                    && rpp::constraint::observable<std::invoke_result_t<Fn, rpp::utils::extract_observable_type_t<TObservable>>>)
+    auto operator()(TObservable&& observable) const &


Can you add test to cover this overloading? (it is enough to apply operator from variable in any test case)

AlexInLog · 2023-06-11T17:52:59Z

Thanks for PR =)

Implement flat_map operator

4f58a84

CorentinBT commented Jun 10, 2023

View reviewed changes

AlexInLog reviewed Jun 10, 2023

View reviewed changes

Address comments

a748a6f

AlexInLog reviewed Jun 10, 2023

View reviewed changes

Corentin and others added 4 commits June 11, 2023 16:21

Address comments

bd37f38

Remove whitespace

47e25dd

Added comments

9e99db8

add marble

aba97fa

AlexInLog reviewed Jun 11, 2023

View reviewed changes

Address comments

627a88e

AlexInLog previously approved these changes Jun 11, 2023

View reviewed changes

AlexInLog reviewed Jun 11, 2023

View reviewed changes

Increase code coverage

45e0ec6

CorentinBT dismissed AlexInLog’s stale review via 45e0ec6 June 11, 2023 16:55

AlexInLog approved these changes Jun 11, 2023

View reviewed changes

AlexInLog merged commit 2e54fc4 into AlexInLog:v2 Jun 11, 2023

		auto obs = rpp::source::create<copy_count_tracker>([verifier = std::move(verifier)](const auto& obs) { obs.on_next(verifier); })
		\| rpp::ops::map([](copy_count_tracker verifier) { return std::move(verifier); }) // copy from source to map

Conversation

CorentinBT commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexInLog Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexInLog Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexInLog commented Jun 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CorentinBT commented Jun 10, 2023 •

edited

Loading

AlexInLog Jun 10, 2023 •

edited

Loading

AlexInLog Jun 10, 2023 •

edited

Loading

codecov Bot commented Jun 10, 2023 •

edited

Loading